Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix (utils): Fix memory leak in EventEmitterMixin #8480

Merged

Conversation

bendemboski
Copy link
Contributor

listenTo() stores the callback under both the _events key and the _listeningTo key, but when invoked with a specific callback, stopListening() was only removing the entry under the _events key, holding the callback in memory.

`listenTo()` stores the callback under both the `_events` key and the `_listeningTo` key, but when invoked with a specific callback, `stopListening()` was only removing the entry under the `_events` key, holding the callback in memory until the emitter object was destroyed.
@jodator jodator self-requested a review November 19, 2020 13:01
@jodator jodator self-assigned this Nov 19, 2020
jodator added a commit that referenced this pull request Nov 20, 2020
Fix (utils): Fix memory leak in EventEmitterMixin. See #8480.
@jodator jodator merged commit a2e5bac into ckeditor:master Nov 20, 2020
@jodator
Copy link
Contributor

jodator commented Nov 20, 2020

Hi @bendemboski! Great catch. We tried to refactor this code once and we failed so any fixes to it are hard to test.

I tried to add a unit test for this but, due to extensive Symbol uses and other tricks I didn't manage to.

Anyway, I've added a reference to this PR to the code so hopefully we could track it in the future.

@bendemboski
Copy link
Contributor Author

I tried to add a unit test for this but, due to extensive Symbol uses and other tricks I didn't manage to.

Haha, yeah, me too. I tried for a bit to see if there was a functional problem, where it still existing under _listeningTo would cause it to be called under certain event firing conditions, but couldn't figure that out either. There is a way to actually write automated tests for leaked objects if you run the tests in Electron and use createIDWeakMap (see this article), but that's probably not worth your while to set up unless you find that you're having ongoing/hard to manage issues with memory leaks.

Anyway, glad the code got merged!

@jodator
Copy link
Contributor

jodator commented Nov 21, 2020

Thanks for the article. Yeah, setting up Electron for this might be overkill. We have simple tests that should detect huge memory leaks using window.performance and window.gc() (Chrome must be run with -js-flags="--expose-gc"). But this method is not precise, and we detect only bigger leaks with this method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants